Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow metadata in register transaction #384

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Conversation

lahsivjar
Copy link
Contributor

The PR updates the /register/transaction endpoint to accept NDJSON. The /register/transaction endpoint can now accept metadata as the first event in NDJSON body and cache it for future calls. A complete list of changes introduced in the PR is as follows:

  1. Change the Content-Type for the /register/transaction endpoint from application/vnd.elastic.apm.transaction+json to application/vnd.elastic.apm.transaction+ndjson.
  2. Add support for deflate and gzip content encoding for the /register/transaction endpoint.
  3. Allow metadata as the first event in the NDJSON body to the /register/transaction endpoint. The metadata is cached and used to send data to APM-Server. The cached metadata is NOT updated by the metadata sent via the intake endpoint.

Related Issue

Closes #352

@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Apr 10, 2023
@basepi
Copy link
Contributor

basepi commented Apr 10, 2023

The cached metadata is NOT updated by the metadata sent via the intake endpoint.

This is slightly concerning to me. We have a mechanism for adding "late-breaking" metadata in the python agent, for metadata that is only available at request time. For now, this should not be a problem -- the only late metadata we add is for lambda, and that metadata will be available when we send the partial transaction.

I just wanted to call it out as something we should explicitly state when we update the spec with partial transaction support.

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to stated changes, haven't reviewed the code.

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality sounds good. I won't have a chance to test this for a little while yet (hopefully this week).

)

const txnRegistrationContentType = "application/vnd.elastic.apm.transaction+json"
const txnRegistrationContentType = "application/vnd.elastic.apm.transaction+ndjson"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it OK to break backwards compatibility here? Is there a concern for using older agents? AFAICS, if older agents attempt to send data to this endpoint, we'll just reject it and lose the partial transaction information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been implemented yet by any agent, in fact python agent is the first one to start implementing it so backward compatibility is not an issue at this point.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@lahsivjar
Copy link
Contributor Author

@trentm @basepi Did you get a chance to test this out? I have done some testing with go-agent and it seems to work as expected but wanted to make sure all requirements are appropriately covered before merging.

@trentm
Copy link
Member

trentm commented Apr 18, 2023

@lahsivjar I have not yet, but I think I may start looking at this tomorrow (as part of elastic/apm-agent-nodejs#3136).

@basepi
Copy link
Contributor

basepi commented Apr 18, 2023

I'm working on testing this today. I was putting it off because I haven't gotten around to building the extension from source yet. 😅

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, working great!

@lahsivjar lahsivjar merged commit 02f4ef1 into elastic:main Apr 19, 2023
@lahsivjar lahsivjar deleted the metadata branch April 19, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-λ-extension AWS Lambda Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension might drop data if the first call for an execution environment fails
4 participants